Cohort22 - Sphinx - TatianaSnook #31
Conversation
There was a problem hiding this comment.
Nice work overall, Tatiana! Though I would like for you to read through the comments to see the ones I left about refactoring. There are a lot of places where you could have DRY'd up your code. Refactoring is a big part of this project because it helps train your eye for seeing similarities in your codebase as well as practicing maintainability and scalability.
| from .models import task, goal | ||
| from .models import task | ||
| from .models import goal | ||
| from .routes.task_routes import tasks_bp |
There was a problem hiding this comment.
Don't forget that the convention for naming blueprints is to name them bp. With each blueprint being named the same thing we will need to import them under an alias like so:
from .routes.task_routes import bp as tasks_bpThough I see you did it for goal
| @classmethod | ||
| def from_dict(cls, goal_data): | ||
| new_goal = cls(title=goal_data["title"]) | ||
| return new_goal |
There was a problem hiding this comment.
Nice work on this file! Straightforward and concise!
| completed_at: Mapped[str | None] = mapped_column(String, nullable=True) | ||
| is_complete: Mapped[bool] = mapped_column(Boolean, default=False) | ||
| goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id")) | ||
| goal: Mapped[Optional["Goal"]] = relationship(back_populates="tasks") |
There was a problem hiding this comment.
I see you mixed a few syntaxes here for the optional columns. I would research to see which one is most preferred and switch them all to that syntax for consistency and convention.
| "description": self.description, | ||
| "is_complete": self.completed_at is not None, | ||
| } | ||
| if include_goal_id and self.goal_id is not None: |
There was a problem hiding this comment.
So the second condition is really the only condition we need to a check for here since if a task doesn't have a goal then goal_id will be None regardless. Also another thing to consider is the consistency of our responses. Sometimes adding a key (goal_id) and sometimes not makes our API less uniform.
|
|
||
| if "title" not in request_body: | ||
| return {"details": "Invalid data"}, 400 | ||
|
|
||
| title = request_body["title"] | ||
|
|
||
| new_goal = Goal(title=title) | ||
| db.session.add(new_goal) | ||
| db.session.commit() | ||
|
|
||
| return {"goal": new_goal.to_dict()}, 201 |
There was a problem hiding this comment.
I see that we have a create_model function in our utilities function, is there a reason as to why we didn't utilize it here?
| description = request_body["description"] | ||
| completed_at = request_body.get("completed_at", None) | ||
| is_complete = request_body.get("is_complete", False) | ||
| goal_id = request_body.get("goal_id", None) |
There was a problem hiding this comment.
We could use the create_model method that we created here as well.
|
|
||
| if sort_order == "asc": | ||
| query = db.select(Task).order_by(Task.title.asc()) | ||
| elif sort_order == "desc": | ||
| query = db.select(Task).order_by(Task.title.desc()) | ||
| else: | ||
| query = db.select(Task).order_by(Task.id) | ||
|
|
||
| tasks = db.session.scalars(query) | ||
| tasks_response = [task.to_dict(include_goal_id=True) for task in tasks] |
There was a problem hiding this comment.
Remember in class we wrote a function called get_model_with_filters though the implementation we had doesn't work in this project, the logic could be modified used here so we could DRY up our code when getting multiple records of a table.
| api_key = os.environ.get("SLACK_BOT_TOKEN") | ||
| headers = {"Authorization": f"Bearer {api_key}"} | ||
| request_body = { | ||
| "channel": "task-notifications", | ||
| "text": f"Task '{task.title}' has been completed!" | ||
| } | ||
|
|
||
| response = requests.post(url, headers=headers, data=request_body) | ||
|
|
||
| if response.status_code != 200: | ||
| return {"error": "Failed to send Slack notification"}, 500 | ||
|
|
||
| return {"task": task.to_dict()}, 200 |
There was a problem hiding this comment.
I would move this logic into a helper function and then that helper function into our utilities file to help with separation of concerns and readability.
| db.session.commit() | ||
|
|
||
| return {"details": f'Task {task_id} "Go on my daily walk 🏞" successfully deleted'} |
There was a problem hiding this comment.
This response shouldn't be for a specific task, it should be dynamic to return the id and description of any arbitrary task.
| response = client.put("/goals/1", json={ | ||
| "title": "My New Goal" | ||
| }) | ||
| response_body = response.get_json() | ||
|
|
||
| # Assert | ||
| # ---- Complete Assertions Here ---- | ||
| # assertion 1 goes here | ||
| # assertion 2 goes here | ||
| # assertion 3 goes here | ||
| # ---- Complete Assertions Here ---- | ||
|
|
||
| assert response.status_code == 200 | ||
| assert "goal" in response_body | ||
| assert response_body == { | ||
| "goal": { | ||
| "id": 1, | ||
| "title": "My New Goal" | ||
| } | ||
| } | ||
|
|
No description provided.